-
Notifications
You must be signed in to change notification settings - Fork 683
Handle negative zero in number multiplication #1856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
It is related to #1855. When I checked it with jerry-test-suite and jerry-tests, there was no regression. And when I run benchmarks 10kk, and 1kk, it also shows no regression on x86_64. Could you check the correctness and run sunspider? |
The bug must be elsewhere. -0 is not an integer number in JerryScript. I suspect the division is the problem. Let me check. |
jerry-core/vm/vm.c
Outdated
@@ -1764,7 +1764,7 @@ vm_loop (vm_frame_ctx_t *frame_ctx_p) /**< frame context */ | |||
&& -(ECMA_INTEGER_MULTIPLY_MAX * ECMA_INTEGER_MULTIPLY_MAX) >= ECMA_INTEGER_NUMBER_MIN, | |||
square_of_integer_multiply_max_must_fit_into_integer_value_range); | |||
|
|||
if (ecma_are_values_integer_numbers (left_value, right_value)) | |||
if (left_value != 0 && right_value != 0 && ecma_are_values_integer_numbers (left_value, right_value)) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it is a bug with the multiplication but we need to move this check down to the if
starting with:
if (-ECMA_INTEGER_MULTIPLY_MAX <= left_integer
Add:
&& left_integer != 0
&& right_integer != 0
Ok. I'll move it. Thank you for review. |
Keep sign for zero. For example, 1 / (0 * (-1)) should be -Infinity, not +Infinity. JerryScript-DCO-1.0-Signed-off-by: Sanggyu Lee [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Keep sign for zero.
For example, 1 / (0 * (-1)) should be -Infinity, not +Infinity.
JerryScript-DCO-1.0-Signed-off-by: Sanggyu Lee [email protected]